-
Notifications
You must be signed in to change notification settings - Fork 7.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208
[USB Serial/JTAG Driver] use time-limited blocking for TX (IDFGH-8776) #10208
Conversation
This PR was reopened due to renaming the branch. |
fa12dd3
to
de6fb9f
Compare
@igrr bump. |
de6fb9f
to
b21b1df
Compare
components/driver/usb_serial_jtag.c
Outdated
|
||
// try to send without blocking | ||
if (xRingbufferSend(sjtag->tx_ring_buf, (void*) (src), size, 0)){ | ||
goto success; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: 4 spaces of indentation, and a space before the opening curly brace on the line above.
(Please try to adjust the code so it looks like the rest of the code nearby.)
components/driver/usb_serial_jtag.c
Outdated
if (xRingbufferSend(sjtag->tx_ring_buf, (void*) (src), size, ticks_to_wait)){ | ||
goto success; | ||
} else { | ||
sjtag->tx_blocking_attempts++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this variable is only ever compared to zero, can it be a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could, but I found this variable name easier to reason about. perhaps: bool tx_has_tried_blocking;
? lmk what you think of that name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_tried_blocking sounds understandable.
@@ -53,26 +53,26 @@ The USB Serial/JTAG Controller is able to put the {IDF_TARGET_NAME} into downloa | |||
Limitations | |||
=========== | |||
|
|||
There are several limitations to the USB console feature. These may or may not be significant, depending on the type of application being developed, and the development workflow. | |||
There are several limitations to the USB Serial/JTAG console feature. These may or may not be significant, depending on the type of application being developed, and the development workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if mentioning JTAG here is less confusing. The JTAG part seems to have nothing to do with the CDC console?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USB console to me sounds ambiguous. Does it also refer to the USB OTG CDC Console?
"USB Serial/JTAG Console" != "USB OTG CDC Console". No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically they are both "CDC", just implemented using two different peripherals. I thought that the title of the document makes it clear which one we are talking about, but maybe someone would land into the middle of the document from a Google search.
Let's keep the changes you made. If i get disagreement about this during the internal review, i will let you know.
|
||
3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different if the ESP-IDF application does not listen for incoming bytes. An USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will block until the application reads the bytes. This can lead to a non-responsive looking terminal program. | ||
3. The behavior between an actual USB-to-serial bridge chip and the USB Serial/JTAG Controller is slightly different. A USB-to-serial bridge chip will just send the bytes to a (not listening) chip, while the USB Serial/JTAG Controller will do a one-time wait of up to 50 milliseconds hoping for the terminal to read the bytes. This can add what appears to be a short "pause" in your ESP-IDF application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the phrase "A USB-to-serial bridge chip will just send the bytes to a (not listening) chip," talks about the data transfer from PC to the ESP. The second part of the sentence now talks about the data transfer from ESP to PC.
I think the original sentence about PC terminal being unresponsive when the ESP doesn't "listen" is still valid here. You can expand the paragraph to explain the delay behavior for the transfer in the other direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it. confusing. maybe I can improve the wording.
Just a few minor notes @chipweinberger, looks good otherwise! |
b21b1df
to
e68d81e
Compare
+1 and thanks all for fixing this as it resolves some usb issues I have seen with the s3. Let me know if there is anything I can do to help push this along. |
sha=e68d81e25518650c7c1e1f9f7cbb2ed26f4128cf |
@igrr, when will this land on master? I've never really understood what |
bump :) |
Closing. Merged. |
Older PR: #10099
Tested working on a ESP32-S3.
Uses Ivan's suggestions for Usb Serial JTAG TX with the driver.